Conversation
AishDani
requested review from
umeshmore45 and
vikrantraut
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This PR refactors parts of the Drupal migration pipeline to rely more on UI-provided content type/field mappings, adds typed config interfaces for Drupal services, and adjusts the migration step sequence (notably removing schema generation calls).
Changes:
- Update Drupal migration flow in
migration.service.ts(removedgenerateContentTypeSchemasstep; updatedcreateEntryinvocation/signature usage). - Refactor Drupal entries processing to use
contentTypes+fieldMappingpassed in (instead of reading LowDB models inside the entries service). - Add Drupal config interfaces and tighten types in
drupal.service.ts; harden Drupal assets/field-analysis services with additional defensive checks/sanitization.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| api/src/services/migration.service.ts | Updates the Drupal migration orchestration (step sequence + updated createEntry args). |
| api/src/services/drupal/interface.ts | Introduces DbConfig and AssetsConfig types for Drupal services. |
| api/src/services/drupal/field-analysis.service.ts | Adds more defensive optional chaining around key-safety and parsing paths. |
| api/src/services/drupal/entries.service.ts | Refactors entry transformation to use passed-in content type mappings; adjusts locale/content-type folder naming. |
| api/src/services/drupal/assets.service.ts | Adds fid/filename sanitization, more defensive checks, and logging improvements in asset export. |
| api/src/services/drupal.service.ts | Removes generateContentTypeSchemas export and introduces typed config usage for Drupal service methods. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const nodeIds = baseEntries?.map((entry) => entry?.nid); | ||
| const fieldsForType = await fieldFetcher?.getFieldsForContentType( | ||
| contentType, | ||
| ); |
There was a problem hiding this comment.
In the optimized-query path, FieldFetcherService expects a Drupal bundle string (contentType: string), but processEntries is now passing the entire contentType object. This makes getFieldsForContentType() return an empty list (because configData.bundle === contentType will never match), so field data won't be fetched/merged and entries will be missing most fields for content types that hit the optimized query flow. Pass contentType.otherCmsUid (the Drupal bundle) to getFieldsForContentType() / fetchFieldDataForContentType() (and keep the object for output UID mapping if needed).
| ): Promise<any[]> => { | ||
| return new Promise((resolve, reject) => { | ||
| connection.query(query, (error, results) => { | ||
| connection?.query?.(query, (error, results) => { |
There was a problem hiding this comment.
executeQuery now uses connection?.query?.(...). If connection is ever null/undefined (or query is missing), the callback will never run and this Promise will never resolve/reject, causing the migration to hang. Since connection is required here, it’s safer to call connection.query(...) directly, or explicitly reject/throw when connection/connection.query is unavailable.
| connection?.query?.(query, (error, results) => { | |
| if (!connection || typeof (connection as any).query !== 'function') { | |
| reject(new Error('Invalid MySQL connection: query method is not available.')); | |
| return; | |
| } | |
| (connection as any).query(query, (error: any, results: any) => { |
No description provided.